fix(integrations): escape multiline/control chars in SKILL.md frontmatter#3392
Open
Quratulain-bilal wants to merge 1 commit into
Open
fix(integrations): escape multiline/control chars in SKILL.md frontmatter#3392Quratulain-bilal wants to merge 1 commit into
Quratulain-bilal wants to merge 1 commit into
Conversation
…tter the hand-built SKILL.md frontmatter in the skills setup path (base.py, and the hermes twin) wrapped values in a double-quoted yaml scalar but only escaped backslash and quote. a multiline (block-scalar) description was then emitted with raw newlines inside the quoted scalar and reparsed with those newlines collapsed to spaces; a control character would break yaml loading outright. the description comes from the template frontmatter, so this is reachable for any command with a block-scalar description. add a shared specify_cli/_yaml_string.quote_yaml_double helper that emits a valid double-quoted scalar (newline/cr/tab short escapes, other control chars as \xXX) and route both renderers through it. added a regression test that round-trips a multiline description through the yaml parser.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes YAML frontmatter generation for skills-based integrations by centralizing proper YAML double-quoted scalar escaping, preventing multiline descriptions and control characters from corrupting or breaking generated SKILL.md files.
Changes:
- Added
specify_cli/_yaml_string.quote_yaml_double()to escape newlines/CR/tab and other control characters into valid YAML escapes. - Switched the manual
SKILL.mdfrontmatter renderers inSkillsIntegrationandHermesIntegrationto use the shared quoting helper. - Added a regression test to ensure multiline template descriptions round-trip through YAML parsing.
Show a summary per file
| File | Description |
|---|---|
| tests/integrations/test_integration_base_skills.py | Adds regression coverage for multiline description round-trip in generated SKILL.md frontmatter. |
| src/specify_cli/integrations/hermes/init.py | Uses shared YAML quoting helper for Hermes skill frontmatter rendering. |
| src/specify_cli/integrations/base.py | Uses shared YAML quoting helper for skills frontmatter rendering in the base skills setup path. |
| src/specify_cli/_yaml_string.py | Introduces a shared, reusable YAML double-quoted scalar escaping helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
Comment on lines
+144
to
+176
| def test_skill_frontmatter_preserves_multiline_description( | ||
| self, tmp_path, monkeypatch | ||
| ): | ||
| """A multiline (block-scalar) description must round-trip exactly. | ||
|
|
||
| The hand-built SKILL.md frontmatter used to only escape backslash and | ||
| quote, so a block-scalar description was emitted with raw newlines inside | ||
| a double-quoted scalar and reparsed with those newlines collapsed to | ||
| spaces. The description must survive byte-for-byte.""" | ||
| i = get_integration(self.KEY) | ||
| template = tmp_path / "sample.md" | ||
| template.write_text( | ||
| "---\n" | ||
| "description: |\n" | ||
| " first line\n" | ||
| " second line\n" | ||
| "scripts:\n" | ||
| " sh: scripts/bash/x.sh\n" | ||
| "---\n" | ||
| "Body\n", | ||
| encoding="utf-8", | ||
| ) | ||
| monkeypatch.setattr(i, "list_command_templates", lambda: [template]) | ||
|
|
||
| m = IntegrationManifest(self.KEY, tmp_path) | ||
| created = i.setup(tmp_path, m) | ||
| skill_files = [f for f in created if f.name == "SKILL.md"] | ||
| assert len(skill_files) == 1 | ||
|
|
||
| content = skill_files[0].read_text(encoding="utf-8") | ||
| fm = yaml.safe_load(content.split("---", 2)[1]) | ||
| assert "\n" in fm["description"] | ||
| assert fm["description"] == "first line\nsecond line\n" |
mnriem
requested changes
Jul 8, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback and resolve conflicts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #3391
the skills setup path builds SKILL.md frontmatter by hand and its
_quotehelper only escaped backslash and quote. a double-quoted yaml scalar can't carry a raw newline or control character, so a multiline (block-scalar) description reparsed with its newlines collapsed to spaces, and a control character broke yaml loading entirely. the description comes from the template frontmatter, so any command with a block-scalar description hits the multiline corruption.this is the SKILL.md sibling of the toml escaping in #3341 and the goose yaml issue #3382 — a different code path in the same file, plus the identical
_quotetwin in the hermes integration.fix: add a shared
specify_cli/_yaml_string.quote_yaml_doublehelper that emits a valid double-quoted scalar (newline/cr/tab short escapes, other control chars as\xXX) and route both renderers through it, so there's one implementation to keep correct.added a regression test that round-trips a multiline description through the yaml parser. i confirmed it fails on the pre-fix code by stashing the source and re-running (
first line\nsecond line->'first line second line '); it passes with the fix, and the skills suites stay green.